KCL executor: Iterate over binary ops (instead of recursing)#6226
KCL executor: Iterate over binary ops (instead of recursing)#6226adamchalmers wants to merge 1 commit intomainfrom
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
adamchalmers
changed the title
adamchalmers
force-pushed
the
achalmers/iterative-binary-ops
branch
from
e1ca04d to
7711bf7
Compare
| let metadata = partial.metadata(); | ||
| partial = do_binary_op( | ||
| partial, | ||
| next_binary_expr.right.get_result(exec_state, ctx).await?, |
There was a problem hiding this comment.
Here and above we're still recursing down the right hand side.
I'm not sure about precedence, that shouldn't be an issue because it should be unambiguous after parsing, but I'm not exactly clear about the order of operations here. Because KCL is mostly immutable, I don't think evaluation order is important, though I would rather it were preserved with this refactoring just in case there is an interesting edge case with tags or something.
I would expect to implement this with basically a manual stack - using a Vec rather than the Rust stack for the KCL call stack. Then it's just an iterative depth-first traversal of the AST and we maintain evaluation order
|
NICE! |
Previously in a member expression like `foo.x` or `foo[3]`, `foo` had to be an identifier. You could not do something like `f().x` (and if you tried, you got a cryptic error). Rather than make the error better, we should just accept any expression to be the LHS of a member expression (aka its 'object'). This does knock our "parse lots of function calls" from 58 to 55 calls before it stack overflows. But I think it's fine, we'll address this in #6226 when I get back to it. Closes #7273
4 participants
Doesn't actually have any performance impact. But it does avoid the stack overflow we were running into.
Restores this add_lots stress test to its glorious full size, undoing the temporary fix of ce7a967 (#6174)
This probably has a bug around operator precedence. Hopefully CI lets me know.